Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

fix: QnA Maker EndpointKey is null Issue#1558

Closed
VanyLaw wants to merge 6 commits intomasterfrom
wenyluo/fix
Closed

fix: QnA Maker EndpointKey is null Issue#1558
VanyLaw wants to merge 6 commits intomasterfrom
wenyluo/fix

Conversation

@VanyLaw
Copy link
Contributor

@VanyLaw VanyLaw commented Nov 12, 2019 •
edited
Loading

Description

This PR is to fix this Issue

Root Cause
This Issue is because the botproject(runtime) read qna.endpointkey from disk before. After we omitted it, we need to send this from composer server to botproject(runtime).

Task Item

closes #1538

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code refactor (non-breaking change which improve code quality, clean up, add tests, etc)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Doc update (document update)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

Screenshots

During the QnaMaker trace, it won't show "Object not set to instance of an object" anymore.
image

@VanyLaw VanyLaw changed the title Wenyluo/fix Fix QnA Maker EndpointKey is null Issue Nov 12, 2019
@a-b-r-o-w-n a-b-r-o-w-n changed the title Fix QnA Maker EndpointKey is null Issue fix: QnA Maker EndpointKey is null Issue Nov 12, 2019
@cwhitten
Copy link
Member

cwhitten commented Nov 12, 2019

The design here is odd. What happens when there are multiple QnA kb's in play? This looks to hardcode the qna key on the settings object.

cwhitten
cwhitten suggested changes Nov 12, 2019
View reviewed changes
Copy link
Member

@cwhitten cwhitten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See earlier comment

@VanyLaw
Copy link
Contributor Author

VanyLaw commented Nov 13, 2019 •
edited
Loading

The design here is odd. What happens when there are multiple QnA kb's in play? This looks to hardcode the qna key on the settings object.

Yes, Thanks for the reminding. I don't have an accurate concept of multiple QnA kb's before. I treat it as a fixed property as luis key. According to @luhan2017 and @hibrenda 's description, I try some corner cases and find it has nothing to do with the property name( no mater it's qna.knowledgebaseid /qna.endpointkey/… or not). So those key-value pairs are more like self-defined property, user who want to use QnA just need to make sure the property in settings is the same as in form.
So, I think it's hard to find a pattern to omit those sensitive properties(such like endpointkey) saving to disk. The best solution for this Issue is simply treated it as self-defined value and not omit it.

FYI. It's a case that works.
image
image

@VanyLaw VanyLaw closed this Nov 13, 2019
@VanyLaw VanyLaw deleted the wenyluo/fix branch November 13, 2019 13:59
@VanyLaw VanyLaw mentioned this pull request Nov 13, 2019
Merged
9 tasks
@VanyLaw
Copy link
Contributor Author

VanyLaw commented Nov 13, 2019

@cwhitten Sorry, I mis-deleted my branch caused this PR closed. I send a new one here

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@a-b-r-o-w-n a-b-r-o-w-n Awaiting requested review from a-b-r-o-w-n a-b-r-o-w-n is a code owner

@boydc2014 boydc2014 Awaiting requested review from boydc2014 boydc2014 is a code owner

@luhan2017 luhan2017 Awaiting requested review from luhan2017

1 more reviewer

@cwhitten cwhitten cwhitten requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@VanyLaw @cwhitten @a-b-r-o-w-n